-
Notifications
You must be signed in to change notification settings - Fork 316
Add append_path() for Url #3161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new append_path() method to the Url type through an extension trait, providing a convenient way to append path segments to URLs while handling slash normalization and preserving query parameters.
- Introduces
UrlExttrait withappend_path()method for intelligent URL path manipulation - Implements slash handling logic to avoid double slashes or missing separators
- Adds comprehensive test coverage for various URL and path combinations
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Heath Stewart <[email protected]>
Co-authored-by: Heath Stewart <[email protected]>
Co-authored-by: Heath Stewart <[email protected]>
Co-authored-by: Heath Stewart <[email protected]>
Co-authored-by: Heath Stewart <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert "overwrite if new path starts with slash"
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
Then why does https://github.com/Azure/typespec-rust/pull/635 change all the paths to start with a preceding slash? I'm not opposed to a helper in core. I suggested something like this (and query param helpers) a while back to @jhendrixMSFT. I've suggested helpers for other things more recently. And I appreciate that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better, but can be more efficient for now (see code suggestion). Before we GA, we should actually look at splitting a path in TypeSpec and building up segments more efficiently using Url::path_segments_mut(). We can then in the emitter efficiently add fixed string segments like "containers" and just append variables to be replaced with the name instead of a String::replace which is terribly inefficient to do repeatedly. The String grows more often and by smaller amounts than probably necessary if more is known up front.
In some code we'll run in tight loops like with Storage Blobs, we're going to be building up endpoints quickly in a tight loop so we'll want to optimize where we can. We can always get better, though, so this doesn't haven't to be perfect. I'd just consider my suggestion below. IMO, it's more intuitive and certainly has fewer allocations. You're will have at least 2 but likely more.
Co-authored-by: Heath Stewart <[email protected]>
Usage in codegen: https://github.com/Azure/typespec-rust/pull/635